Skip to content

security: add allowed_classes => false to SequenceGenerator::unserialize()#12492

Open
XananasX7 wants to merge 1 commit into
doctrine:3.6.xfrom
XananasX7:security/sequence-generator-unserialize
Open

security: add allowed_classes => false to SequenceGenerator::unserialize()#12492
XananasX7 wants to merge 1 commit into
doctrine:3.6.xfrom
XananasX7:security/sequence-generator-unserialize

Conversation

@XananasX7
Copy link
Copy Markdown

Summary

The deprecated Serializable::unserialize() method in SequenceGenerator calls unserialize($serialized) without an allowed_classes restriction.

// src/Id/SequenceGenerator.php  (before)
$this->__unserialize(unserialize($serialized));

The serialized payload is always the output of __serialize(), which returns only two scalar values:

['allocationSize' => $this->allocationSize, 'sequenceName' => $this->sequenceName]

No object classes are expected. Without allowed_classes => false, a crafted serialized string supplied to the deprecated method could instantiate arbitrary autoloaded classes during deserialization (PHP Object Injection), potentially triggering a gadget chain.

Fix

$this->__unserialize(unserialize($serialized, ['allowed_classes' => false]));

This is safe because __unserialize() only reads $data['sequenceName'] (string) and $data['allocationSize'] (int).

Notes

  • The Serializable interface and its methods are already deprecated; this is a defense-in-depth fix to harden the remaining code path until it is removed in ORM 4.
  • No behaviour change for legitimate data.
  • The modern __serialize() / __unserialize() magic methods are not affected.

…ize()

The deprecated Serializable::unserialize() method calls unserialize($serialized)
without an allowed_classes restriction. The serialized data is always a plain
associative array of two scalar values (sequenceName: string, allocationSize: int)
produced by __serialize(); no object classes are expected.

Adding allowed_classes => false prevents PHP Object Injection if a crafted
serialized payload is supplied to the deprecated unserialize() method, which
could trigger a gadget chain during deserialization.
@beberlei
Copy link
Copy Markdown
Member

Its virtually impossible that this happens in real applications as it would require user supplied entity mappings, but we can merge this regardless to avoid machines that are not able to make this conclusion from reporting this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants